Here's a preliminary benchmark done with hyperfine running on my computer with as little other programs running as possible, done in the same condition (beside being run 6 hours apart).
It seems like we are within the standard deviation, so there's no noticeable degrade of performance.

Upstream:

~/r/haskell/cabal λ lts-18.28
$ hyperfine --runs 30 './validate.sh --partial-hackage-tests'
Benchmark 1: ./validate.sh --partial-hackage-tests
  Time (mean ± σ):     203.400 s ± 21.816 s    [User: 150.484 s, System: 39.487 s]
  Range (min … max):   183.805 s … 277.905 s    30 runs

This branch:

…/wt/haskell/cabal/exact-pp-leana λ lts-18.28
$ hyperfine --runs 30 './validate.sh --partial-hackage-tests'
Benchmark 1: ./validate.sh --partial-hackage-tests
  Time (mean ± σ):     199.168 s ± 10.540 s    [User: 156.373 s, System: 39.818 s]
  Range (min … max):   184.443 s … 242.563 s    30 runs

Thank you for your response Andrea, I'll write up a response and get back to you soon :)

Thank you for your comment @andreabedini :)

I think this has been already discussed before: we have the ann parameter which can be used to keep hold on the comments.

That looks very interesting, but how would I deal with files that are just comments? To the point of view of readFields they should be valid yet we would have no Field to attach them to. Whether we should attach the comments above or below is yet another question. For example, in the sequence "comment element comment element comment", which element should grabs the comment in between?

I do think your model is very interesting so if you have the time to, please show working PR against mine so we can simply merge it in 🙏

This is already the practice of few packages developed by the community. Is there a reason to deviate from this?

Could you elaborate which packages are these? I would love to have more insight on how people solve similar problems.

Are there other design issues that needs to be addressed ?

That looks very interesting, but how would I deal with files that are just comments?

Are they valid Cabal files if there is nothing but comments? I don't think so.

Could you elaborate which packages are these? I would love to have more insight on how people solve similar problems.

For instance, cabal-add has a function

annotateFieldsWithSource :: ByteString -> [Field Position] -> [Field ByteString]

which annotates each field with its source including all adjacent comments. It can be modified to return [Field (Position, ByteString) if you need both.

Are they valid Cabal files if there is nothing but comments? I don't think so.

Pretty sure you need at minimum name and one target.

Hi friends, thanks for all your responses. Leana needs some time to read up on the exact proposal to see how it all fits together before replying.
After chatting with her, I think she wants to go with Andrea's design for the comment field parser. As you can see, she's deeply in the weeds about many of the details of the parser; she even corrected some of the field grammar comments!
I don't know what you guys think, but I think it's good progress 🚀

Cabal is one of the toughest code bases I ever worked on, so I'm quite amazed by Leana making progress so quickly!

Thank you Bodigrim and Jappie for your kind words! That means a lot to me, I'm glad to be on the right track.

I have started (and completed) to rewrite my PR using Andrea's approach.
Right now the behaviour is identical to my old approach, while comments are tracked in the annotation ann.

Are they valid Cabal files if there is nothing but comments? I don't think so.

Good to know. Currently the top level parser drops the comments consumed if there are no fields to attach them to.

Let me know what you think about the change :)

Here are the benchmark results. The baseline has been rerun because I did these ones on a VPS machine, and they are not comparable to the last ones I ran on my machine.

# baseline
leana@Ubuntu-2404-noble-amd64-base:~/cabal$ hyperfine './validate.sh --partial-hackage-tests'
Benchmark 1: ./validate.sh --partial-hackage-tests
  Time (mean ± σ):     253.353 s ±  9.520 s    [User: 196.642 s, System: 60.881 s]
  Range (min … max):   241.649 s … 271.207 s    10 runs
  
# this PR
leana@Ubuntu-2404-noble-amd64-base:~/cabal$ hyperfine --setup "./validate.sh --partial-hackage-tests" "./validate.sh --partial-hackage-tests"
Benchmark 1: ./validate.sh --partial-hackage-tests
  Time (mean ± σ):     253.163 s ±  7.451 s    [User: 196.432 s, System: 58.507 s]
  Range (min … max):   239.373 s … 266.656 s    10 runs

@leana8959

That looks very interesting, but how would I deal with files that are just comments?

In my prototype I have replaced [Field ann] with something like

data File ann = File [Field ann] ann

Where the extra annotation is for anything coming after the last field.

Could you elaborate which packages are these? I would love to have more insight on how people solve similar problems.

In addition to @Bodigrim's cabal-add, @phadej's cabal-fields rewrites parser entirely (but dropping support for braces) but at the AST level does the same thing. I am sure he also left comments in some of the "exact printing" mega-threads.

After chatting with her, I think she wants to go with Andrea's design for the comment field parser. As you can see, she's deeply in the weeds about many of the details of the parser; she even corrected some of the field grammar comments!

I am available to discuss and support her effort. @leana8959 I'll reach out privately.

Cabal is one of the toughest code bases I ever worked on, so I'm quite amazed by Leana making progress so quickly!

I warmly second this!

@leana8959, @andreabedini: how is the private communication going? We are interested too! Could we help somehow?

for clarity: These parser changes are ready for review as far as leana and me are concerned, meanwhile we've moved over to import stanza retention in GenericPackageDescription (they currently get merged into the stanza's). This is an independent change of the parser changes here.

After that we can start on exact print propper.

import stanza retention in GenericPackageDescription (

Please don't. A lot of code wants an elaborated (= stripped down of syntactic convenience) representation of package description, and GPD serves that now.

Your parsing changes leak down the pipeline where they shouldn't. E.g. things like solver works with GPD, and it really shouldn't care about whether import stanzas were used to declare a package or not.

This PR isn't about that,
and we're intending to keep everything working. 🙂

This PR does add

  , exactComments :: ExactComments Position

field to GPD. I don't see a point of having comments in GPD. (As noted in tests, it "breaks" equality)

@phadej Indeed, I have added a newtype around GenericPackageDescription that doesn't have an Eq instance. This ensures GenericPackageDescription stays the same :)

I understand, the authors would like to get reviews for this PR. If this is so, please, squash the commit history. For the size of PR: a good part of the changes are test-suite changes, it seems. I don't think they have to be extracted into separate PR or even separate commits (because having commits that don't pass CI individually may be cumbersome in the future, for git-bisecting and alike).

@mpickering can you take a high-level look at the design in this PR and tell us your opinion? The gist of it is:

-- Cabal-syntax/src/Distribution/Fields/Field.hs

data Comment ann = Comment !ByteString !ann
  deriving (Show, Generic, Eq, Ord, Functor)

data WithComments ann = WithComments
  { justComments :: ![Comment ann]
  , unComments :: !ann
  }
  deriving (Show, Generic, Eq, Ord, Functor)

and then many parsing utilities that used to return ... Field Position now return Field (WithComments Position). With casual renamings like below:

-parseGenericPackageDescription'
+parseAnnotatedGenericPackageDescription'
  :: Maybe CabalSpecVersion
  -> [LexWarning]
  -> Maybe Int
-  -> [Field Position]
-  -> ParseResult src GenericPackageDescription
-parseGenericPackageDescription' scannedVer lexWarnings utf8WarnPos fs = do
+  -> [Field (WithComments Position)]
+  -> ParseResult src AnnotatedGenericPackageDescription
+parseAnnotatedGenericPackageDescription' scannedVer lexWarnings utf8WarnPos fs = do

One particular thing that bothers me is that clients will have to call unComment even if they don't care about comments (at least, it appears to be the case to me after looking at the diff in this patch). I feel like there should be a more graceful way to handle this. On the cabal meeting today @jappeace suggested that we could duplicate (I assume, some) parsing utilities so that some functions have two versions: one comment-bearing and another one, comment-less. I must say I quite like this idea. @geekosaur pointed out that this "just" moves the pressure from client's to our shoulders. But I feel like many clients aren't interested in comments anyway (they are surviving somehow today), and the cost of this sort of contained duplication (inside one module and very uniform) is low. So, overall, the cost/benefit ratio of this duplication-based idea is better than in the current proposal to me.

Any thoughts?

Meta-comment: I looked through the tech proposal again, and there doesn't appear to be a technical description of a solution for this particular comments issue. It is totally fine, because the proposal would turn into a foliant at that level of detail. Neveretheless, I wish that, before doing all the technical work in this PR, the authors had discussed the actual technical solution for the particular issue (like preserving comments; others are listed in the tech proposal) on the Cabal bug tracker (here).

If this is so, please, squash the commit history. For the size of PR: a good part of the changes are test-suite changes, it seems. I don't think they have to be extracted into separate PR or even separate commits.

@ulysses4ever Are there some rule of thumb to squash my commits? I already went through the history once this morning and split out all changes that touch the testsuite into one commit.
Do I need to do something else for the feature commits? If it's all good please let me know, I'll force push to my PR branch.
Maybe you have an example PR that does it right?

I cleaned up everything and made sure the test passes, this is ready for review!

@jappeace @leana8959 should we remove the needs-review label for now given #11227 (comment) ?

yes 😄

A checklist on what has been discussed in this thread and their conclusions based on the current design and implementation.

• Andrea: Don't add a new Comment constructor
  -> use ann type variable to store comments

• Bodigrim: Outline the usage of exactComments
  -> for now it extracts the comments annotation away from [Field (WithComments Position)], leaving [Field Position].
  The extracted Comments are made available further down the pipeline, and will be used by the following PRs.

• phadej: Putting comments in GPD will break extensionality of Eq instance.
  -> we separate the comments and they are not stored in the GPD.

• ulysses4ever: One particular thing that bothers me is that clients will have to call unComment
  even if they don't care about comments (at least, it appears to be the case to me after looking at
  the diff in this patch).
  -> jappeace: suggested that we could duplicate (I assume, some) parsing utilities so that some
  functions have two versions: one comment-bearing and another one, comment-less.

  -> leana: it would be possible to define the more complex one that has comments normally. Then, the one that without comments is defined as the commented one with the comments stripped away. These functions should not be complex to maintain, and saves downstream from managing comments.

Bodigrim: Outline the usage of exactComments

-> for now it extracts the comments annotation away from [Field (WithComments Position)], leaving [Field Position].
The extracted Comments are made available further down the pipeline, and will be used by the following PRs.

(NB: I assume Bodigrim meant exTRactComments)

I'm sorry to say but this outline is not helpful to me. Can you come up with concrete examples of what separated comments may be useful for? Because to me (like Bodigrim, I assume) they seem useless.

Same goes for parseCommentedGenericPackageDescription, I assume.

It'd be good to have some indication how this work connects to the more general exact-print work (concretely, #11690 or #11814, whichever is alive; btw it'd be good to garbage collect at some point).

Can you come up with concrete examples of what separated comments may be useful for? Because to me (like Bodigrim, I assume) they seem useless.

@ulysses4ever thanks a lot for the review!
Right now we expose the comment-preserving parsing functions because they might be useful for people who want to test it out. I could mark these functions as "unstable", what do you think?

As a different approach, we can also hide this from the interface for now. This PR will still have its value because it makes the parser ready for future exact print PRs even though this feature is not yet exposed via the interface.

I plan to feed the Fields (WithComments Position) to ParsecFieldGrammar and populate each field with their own associated comments.

 data ParsecFieldGrammar s a = ParsecFG
   { fieldGrammarKnownFields :: !(Set FieldName)
   , fieldGrammarKnownPrefixes :: !(Set FieldName)
-  , fieldGrammarParser :: forall src. (CabalSpecVersion -> Fields Position -> ParseResult src a)
+  , fieldGrammarParser :: forall src. (CabalSpecVersion -> Fields (WithComments Position) -> ParseResult src a)
   }
   deriving (Functor)

src

This will allow printing each field in field grammar to be self contained, and easier to reason with. We also avoid the difficulty of correlating comments with modified fields, which was one of the big blockers in the trivia tree approach #11425.

I'm probably way late to the party, but I'm wondering whether it is morally correct to attach comments to fields in the first place.

AFAIK, the cabal file format does not explicitly specify that comments are supposed to be associated with whatever non-comment follows; by convention, most "real" comments in practice are, but when, for example, you comment out fields or sections in a cabal file, this is almost certainly not the case (you don't think of a commmented-out field as being associated with whatever field happens to come after it). The fact that comments can occur at the end of a cabal file and need to be handled specially there is, IMO, a symptom of this, and my gut feeling says that this probably means that associating comments with fields like this is not the right design.

@tdammers

you comment out fields or sections in a cabal file, this is almost certainly not the case (you don't think of a commmented-out field as being associated with whatever field happens to come after it).

Why not? Note that currently cabal doesn't retain any comments. It is very "correct" because it avoids answering the question, but it also doesn't let us print out the comments.

Making comments a proper construction on par with Field and Sections was the previous design. However, as andrea bedini pointed out, it modifies the meaning of the type.

Also, distinguishing what comment is a real comment is not in the scope of this PR. We don't do haddock-like generation where we need to know to which AST node the comment is attached to. This PR is only concerned with storing the comments properly and passing them on to be printed back out completely.

The fact that comments can occur at the end of a cabal file and need to be handled specially there is, IMO, a symptom of this, and my gut feeling says that this probably means that associating comments with fields like this is not the right design.

(Just to make sure we are on the same page, can you point to me which part of the diff you are talking about?)

I think this situation occurs in every language that allows you to put a comment before and after a line. The real problem is that when the cabal file is only comments, we'd have no fields to attach them to. This is discussed here and we agreed that we'd only support valid cabal files, which will have a non null amount of fields/sections of fields.